WIP: VHDS: on-demand updates #6552

dmitri-d · 2019-04-10T23:25:57Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2019-04-10T23:27:58Z

@htuch: the internal redirect is here: https://github.com/envoyproxy/envoy/pull/6552/files#diff-be24bc0fe90bba5c4e871fb56928e085R48

dmitri-d · 2019-04-10T23:35:08Z

@brian-avery

htuch

Nice, will focus on OnDemandUpdate for until we have the base patches merged.

include/envoy/router/rds.h

source/common/router/on_demand_update.cc

stale · 2019-04-19T21:13:07Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-04-26T21:21:24Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-05-03T21:40:07Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-05-10T22:14:43Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

stale · 2019-05-23T23:52:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…d docs Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2019-05-30T17:29:54Z

@htuch: I think this is ready to be reviewed. Please let me know if you would like me to squash the commits (I think they'll need to be cleaned up, as the sequence of commits with respect to the base VHDS commit is out of order).

htuch

Overall structure/approach looks good, a few comments, would be great to cleanup various commented code and to generally add more docs/comments. Thanks!

htuch · 2019-06-03T16:41:13Z

include/envoy/http/filter.h

@@ -150,6 +150,8 @@ class StreamFilterCallbacks {
   */
  virtual Router::RouteConstSharedPtr route() PURE;

+  virtual bool requestRouteConfigUpdate(std::function<void()> cb) PURE;


Can you add Doxygen docs for this?

htuch · 2019-06-03T16:41:35Z

include/envoy/config/subscription.h

@@ -79,6 +80,10 @@ class Subscription {
   * be passed to std::set_difference, which must be given sorted collections.
   */
  virtual void updateResources(const std::set<std::string>& update_to_these_names) PURE;
+
+  virtual void updateResourcesViaAliases(const std::set<std::string>&) {


Can you add Doxygen docs for this?

If I am perceiving the intention correctly, I think you might also want to change the name. updateResources() is kind of misnamed; it's not about getting updated resources, but rather updating the set of resource names we care about. Similarly, it looks like this one is about adding to a list of aliases, not updating resources.

...so I guess I'm really saying now would be a good time to give updateResources() a better name. Can I suggest something like either updateResourceInterest or updateSubscriptionInterest?

As for the aliases one, since it looks like it's more of a blanket addition than a "replace with this set" (which is how updateResources currently works), I would suggest like addAliases or addResourceAliases.

htuch · 2019-06-03T16:42:35Z

include/envoy/router/rds.h

@@ -48,6 +48,17 @@ class RouteConfigProvider {
   * Callback used to notify RouteConfigProvider about configuration changes.
   */
  virtual void onConfigUpdate() PURE;
+
+  /**
+   * Callback used to request an update to the route configuration.


An update from the management server?

htuch · 2019-06-03T16:43:08Z

include/envoy/router/rds.h

+   * must match on
+   * @param cb callback to be called when the configuration update has been propagated to worker
+   * threads
+   * @return whether a request for a configuration update has been successfully scheduled


How/why can this fail?

htuch · 2019-06-03T16:43:48Z

include/envoy/router/rds.h

+
+  /**
+   * Callback used to request an update to the route configuration.
+   * @param for_domain supplies the domain name that virtual hosts contained in the VHDS response


This is a bit confusing, as it is an update request, but the thing we're requesting is referencing the response..

htuch · 2019-06-03T16:49:51Z

source/common/router/vhds.cc

@@ -48,6 +48,11 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info,
      factory_context.api());
 }

+void VhdsSubscription::ondemandUpdate(const std::set<std::string>& aliases) {
+  std::cout << "11111111111111111111111111111111111\n";


htuch · 2019-06-03T16:49:57Z

source/common/router/router.cc

+        }
+
+        attempting_internal_redirect_with_complete_stream_ = false;
+    */


htuch · 2019-06-03T16:50:32Z

source/common/router/rds_impl.cc

+    return false;
+  }
+  factory_context_.dispatcher().post(
+      [this, for_domain]() -> void { subscription_->ondemandUpdate({for_domain}); });


This should be onDemandUpdate..

There's an issue of collision with Envoy callback naming, though, right? onDemandUpdate sounds like a function that handles a "demand update". Not sure that there's a good solution here, hahaha....

Maybe updateOnDemand? lazyUpdate?

htuch · 2019-06-03T16:51:57Z

source/common/router/rds_impl.cc

+  tls_->runOnAllThreads([this, new_config]() -> void {
+    tls_->getTyped<ThreadLocalConfig>().config_ = new_config;
+    auto callbacks = config_update_callbacks_->getTyped<ThreadLocalCallbacks>().callbacks_;
+    if (!callbacks.empty()) {


Should this be a while? What if there are multiple updates requested before a response is received? Do you have tests for this?

htuch · 2019-06-03T16:53:05Z

source/extensions/filters/http/on_demand/on_demand_update.cc

+
+  if (!callbacks_->decodingBuffer() && // Redirects with body not yet supported.
+      callbacks_->recreateStream()) {
+    // cluster_->stats().upstream_internal_redirect_succeeded_total_.inc();


fredlas

Looks like this fits in with both the current subscription state stuff, and my ongoing work, very nicely; basically zero friction. That's a relief! I also like where the refactoring in DeltaSubscriptionState is going.

fredlas · 2019-06-03T20:33:54Z

include/envoy/config/subscription.h

@@ -79,6 +80,10 @@ class Subscription {
   * be passed to std::set_difference, which must be given sorted collections.
   */
  virtual void updateResources(const std::set<std::string>& update_to_these_names) PURE;
+
+  virtual void updateResourcesViaAliases(const std::set<std::string>&) {


If I am perceiving the intention correctly, I think you might also want to change the name. updateResources() is kind of misnamed; it's not about getting updated resources, but rather updating the set of resource names we care about. Similarly, it looks like this one is about adding to a list of aliases, not updating resources.

...so I guess I'm really saying now would be a good time to give updateResources() a better name. Can I suggest something like either updateResourceInterest or updateSubscriptionInterest?

As for the aliases one, since it looks like it's more of a blanket addition than a "replace with this set" (which is how updateResources currently works), I would suggest like addAliases or addResourceAliases.

fredlas · 2019-06-03T20:41:42Z

source/common/config/delta_subscription_state.cc

+}
+
+void DeltaSubscriptionState::populateDiscoveryRequest(
+    envoy::api::v2::DeltaDiscoveryRequest& request) {


You may be able to factor out the if (!any_request_sent_yet_in_current_stream_) block from populateDiscoveryRequest into its own function. Then, rather than having ifs+elses in getNextRequest(), you could just do

if (!any_request_sent_yet_in_current_stream_) prepareFirstRequest(request); if (!aliases_added_.empty()) populateWithAliases(request); populateDiscoveryRequest(request);

fredlas · 2019-06-03T20:44:21Z

source/common/config/delta_subscription_state.cc

@@ -76,10 +76,15 @@ void DeltaSubscriptionState::updateResourceInterest(
  }
 }

+void DeltaSubscriptionState::updateResourceInterestViaAliases(
+    const std::set<std::string>& updates_to_these_aliases) {
+  aliases_added_.insert(updates_to_these_aliases.begin(), updates_to_these_aliases.end());


One surface-level (since I don't know how it's supposed to work) question to double check you're doing what you intend: is it your intention that aliases are only ever added, not removed? Or, it looks like the actual mechanism is that these aliases just go into resource_names_subscribe; does that mean that to remove them, you are supposed to just do a normal unsubscribe of those names?

fredlas · 2019-06-03T20:49:54Z

source/common/router/rds_impl.cc

+    return false;
+  }
+  factory_context_.dispatcher().post(
+      [this, for_domain]() -> void { subscription_->ondemandUpdate({for_domain}); });


There's an issue of collision with Envoy callback naming, though, right? onDemandUpdate sounds like a function that handles a "demand update". Not sure that there's a good solution here, hahaha....

stale · 2019-06-10T21:22:16Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-06-17T22:18:37Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Dmitri Dolguikh added 14 commits April 3, 2019 12:16

Base implementation of VHDS

f66b55a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

additional formatting fixes

0e6c97e

More formatting fixes

b447f33

more formatting fixes

2da973f

Fixing test failures

21bb7af

wrapped update details into a dedicated class

2a6c77f

fixing build failures

dbafd58

Fixing build failures

200aa3d

Added more vhds tests

68d7c52

Fixed formatting errors

e692f0e

fixes based on the PR feedback

38da6ab

fixed a spelling mistake

9a14662

moved out VhdsSubscription class into its own file

33f795b

VHDS: Filter-based on-demand RDS updates

7a40f59

dmitri-d changed the title ~~VHDS: on-demand updates~~ WIP: VHDS: on-demand updates Apr 10, 2019

htuch suggested changes Apr 12, 2019

View reviewed changes

include/envoy/router/rds.h Outdated Show resolved Hide resolved

source/common/router/on_demand_update.cc Outdated Show resolved Hide resolved

source/common/router/on_demand_update.cc Outdated Show resolved Hide resolved

source/common/router/on_demand_update.cc Outdated Show resolved Hide resolved

htuch reviewed Apr 12, 2019

View reviewed changes

source/common/router/on_demand_update.cc Outdated Show resolved Hide resolved

mattklein123 assigned htuch Apr 12, 2019

htuch added the waiting label Apr 15, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 19, 2019

stale bot closed this Apr 26, 2019

htuch reopened this Apr 26, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 26, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 3, 2019

htuch reopened this May 16, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 16, 2019

Moved on-demand filter to extensions/filters/http/on_demand dir

374de2c

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only bot removed the waiting label May 16, 2019

Dmitri Dolguikh added 2 commits May 16, 2019 15:52

Merge branch 'master' into vhds-on-demand

1c8c52f

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

fix to build kafka extension under python3

c5e314a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch added the waiting label May 20, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 23, 2019

Dmitri Dolguikh added 2 commits May 29, 2019 12:02

Merge branch 'master' into vhds-on-demand

28e6ad3

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Post-merge fixes

bd11db6

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 29, 2019

repokitteh-read-only bot removed the waiting label May 29, 2019

Dmitri Dolguikh added 8 commits May 29, 2019 13:42

Renamed requestRouteConfigUpdate to requestVirtualHostsUpdate, update…

bf63b6d

…d docs Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Added comments re: stream restart after route config update

bbc38b4

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing build issues

01eca26

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing build issues

72896bb

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing build issues

2592cce

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed formatting issues

e5d9d94

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing build issues

7bab936

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixing build issues

852c51b

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch suggested changes Jun 3, 2019

View reviewed changes

htuch added the waiting label Jun 3, 2019

fredlas reviewed Jun 3, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 10, 2019

stale bot closed this Jun 17, 2019

dmitri-d deleted the vhds-on-demand branch May 20, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: VHDS: on-demand updates #6552

WIP: VHDS: on-demand updates #6552

dmitri-d commented Apr 10, 2019

dmitri-d commented Apr 10, 2019

dmitri-d commented Apr 10, 2019

htuch left a comment

stale bot commented Apr 19, 2019

stale bot commented Apr 26, 2019

stale bot commented May 3, 2019

stale bot commented May 10, 2019

stale bot commented May 23, 2019

dmitri-d commented May 30, 2019

htuch left a comment

htuch Jun 3, 2019

htuch Jun 3, 2019

fredlas Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

fredlas Jun 3, 2019

fredlas Jun 3, 2019

htuch Jun 3, 2019

htuch Jun 3, 2019

fredlas left a comment

fredlas Jun 3, 2019

fredlas Jun 3, 2019

fredlas Jun 3, 2019

fredlas Jun 3, 2019

stale bot commented Jun 10, 2019

stale bot commented Jun 17, 2019

WIP: VHDS: on-demand updates #6552

WIP: VHDS: on-demand updates #6552

Conversation

dmitri-d commented Apr 10, 2019

dmitri-d commented Apr 10, 2019

dmitri-d commented Apr 10, 2019

htuch left a comment

Choose a reason for hiding this comment

stale bot commented Apr 19, 2019

stale bot commented Apr 26, 2019

stale bot commented May 3, 2019

stale bot commented May 10, 2019

stale bot commented May 23, 2019

dmitri-d commented May 30, 2019

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredlas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 10, 2019

stale bot commented Jun 17, 2019